-
Notifications
You must be signed in to change notification settings - Fork 394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add missing generic on Factory
subclasses
#1060
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that!
That's a good point for the abstract factories; for the concrete ones (StubFactory
, ListFactory
, DictFactory
), I believe we should use the actual class instead of the typevar'd.
Finally, would you be able to add a couple of tests in tests/test_typing.py
? Those are exercised through make test
as the first check.
Thanks!
Good catch on concrete factories. I updated tests to make use of |
I add tried to use |
Yes this is why I added |
Hi @rbarrois, any chance we could get a release once this is merged? The typing addition from the previous PR would be really useful in test code as it really plays well in IDEs :) Thanks! |
from sqlalchemy.exc import IntegrityError | ||
from sqlalchemy.orm.exc import NoResultFound | ||
|
||
from . import base, errors | ||
|
||
T = TypeVar("T") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it required to create a new TypeVar here? Can't we import it from base.py
or user base.T
? The same applies todjango.py
and mogo.py
.
Following #1059, subclasses were missing the type var, making it impossible to parametrize at runtime.
There's a couple places where I'd like to see typing added (e.g.
LazyAttribute
and friends' callables arguments). They should be pretty straightforward, would you like to have a PR implementing it?